Code Review: PR #635

Verdict: REQUEST CHANGES

Summary: This PR extracts duplicated recording-stop snapshot logic from stopRecording() and cancelRecording() into a private RecordingStopSnapshot struct and makeRecordingStopSnapshot() factory. The deduplication is well-motivated — both paths had identical 6-variable capture blocks — and the refactoring is clean. However, one important ordering comment was dropped, and there's a minor struct design nit.

Blockers (must fix before merge)

None. No correctness, security, or data-loss issues.

Should fix (strongly recommended)

1. Restore the removed ordering comment (MeetingSessionController.swift, was at ~line 521 in the old code)

The original code had this comment:

// Snapshot capture health BEFORE stop, since the system-audio backend
// can clean up buffer counters before file-close completion resumes.

This was removed in the refactoring. The comment documents a non-obvious ordering constraint — the snapshot must be captured before await capture.stopAndAwaitFiles() because the system-audio backend cleans up buffer counters during file-close. Without this comment, a future developer might reasonably move makeRecordingStopSnapshot() to after the stop call (e.g. "we don't need the snapshot until the logging below, so let's move it down").

Suggested fix — add a doc comment on the factory method:

/// Snapshot capture health before the stop call, since the system-audio
/// backend can clean up buffer counters before file-close completion resumes.
private func makeRecordingStopSnapshot() -> RecordingStopSnapshot {

Or alternatively, keep an inline comment at each call site. The factory doc comment is preferred since it protects both callers and centralizes the rationale.

Nits (take or leave)

1. durationMilliseconds could be a computed property

durationMilliseconds is trivially derived from durationSeconds:

private struct RecordingStopSnapshot {
    // ...
    let durationSeconds: TimeInterval
    let durationMilliseconds: Int  // ← always Int(durationSeconds * 1000)
    // ...
}

A computed property eliminates the redundant stored value:

var durationMilliseconds: Int { Int(durationSeconds * 1000) }

This is minor — the struct is private and the factory correctly derives it — but a computed property makes the relationship explicit and removes a potential consistency hazard if someone later constructs the struct manually with mismatched values.

What looks good

• Correct deduplication: The two paths (stopRecording and cancelRecording) had near-identical snapshot capture blocks. Extracting them into a single factory is the right call.
• Subtle bug fix in cancelRecording: The original cancel path computed durationSeconds as Double(durationMs) / 1000, which round-tripped through Int and lost sub-millisecond precision. The new code uses recordingSnapshot.durationSeconds directly. Nice catch (intentional or not).
• Consistent reads: The original stopRecording read recordingDuration twice at different points (once inline in the first log, once as finalRecordingDuration). The new code captures it exactly once, eliminating potential timing skew from a continuously-updating timer.
• Scoping: The struct and factory are both private, keeping the abstraction tightly scoped to where it's needed.
• No efficiency regression: Same operations, same call count. The struct bundling adds negligible overhead for a once-per-meeting-stop operation.
• Correct systemAudioStatus threading: The factory captures systemAudioStatus once and passes it as the override to both healthInfo() and pipelineDiagnosticsSnapshot(), preserving the original consistent-override pattern.

Generated by Claude Code